Add validation for example target paths with clearer error messages#16329
Add validation for example target paths with clearer error messages#16329TanmayArya-1p wants to merge 2 commits intorust-lang:masterfrom
Conversation
tests/testsuite/bad_config.rs
Outdated
| .build(); | ||
| p.cargo("check --example fuzz") | ||
| .with_status(101) | ||
| .with_stderr_contains("[ERROR] couldn't read `examples/fuzz`: Is a directory (os error 21)") |
There was a problem hiding this comment.
I'm assuming there isn't a reason to use a _contains test here. Where possible, we prefer sing our snapshot tests (see above for str![]). This makes it easy to update the output on changes
| // Validate if the path has a .rs extension | ||
| if let Some(extension) = path.extension() { | ||
| if extension != "rs" { | ||
| anyhow::bail!( | ||
| "{} `{}` has path `{}` which does not have a `.rs` extension", | ||
| target_kind, | ||
| target_name, | ||
| path.display() | ||
| ); | ||
| } | ||
| } else { | ||
| anyhow::bail!( | ||
| "{} `{}` has path `{}` which has no file extension (expected `.rs`)", | ||
| target_kind, | ||
| target_name, | ||
| path.display() | ||
| ); | ||
| } |
There was a problem hiding this comment.
What happens today when you have an extension-less file or a non-.rs extension? If it works, then we need to have the conversation about what the intended behavior should be, whether that is a bug or not (and should likely be moved out of this PR to not block on that). If not, we should have tests to compare with to see how the error changed.
There was a problem hiding this comment.
today it works if the example and its path is explicitly mentioned in Cargo.toml. It however does not work if there is an extensionless file, say fuzz in the examples directory and i try cargo check --example fuzz which on hindsight makes sense.
| if !path.exists() { | ||
| anyhow::bail!( | ||
| "can't find {} `{}` at path `{}`", | ||
| target_kind, | ||
| target_name, | ||
| path.display() | ||
| ); | ||
| } |
There was a problem hiding this comment.
We should have a test for this case as well. This might also represent a breaking change.
| let target_name = name_or_panic(&toml); | ||
|
|
||
| validate_target_path_as_source_file(&path, target_name, TARGET_KIND_HUMAN_EXAMPLE)?; |
There was a problem hiding this comment.
As I mentioned on #10173, we shouldn't limit ourselves to examples
| if path.is_dir() { | ||
| anyhow::bail!( | ||
| "path `{}` for {} `{}` is a directory, but a source file was expected.\n\ | ||
| Consider setting the path to the intended entrypoint file instead: `{}/.../main.rs`", |
There was a problem hiding this comment.
What if we talked the directory to report concrete lib.rs and main.rss they could point to?
| if path.is_dir() { | ||
| anyhow::bail!( | ||
| "path `{}` for {} `{}` is a directory, but a source file was expected.\n\ | ||
| Consider setting the path to the intended entrypoint file instead: `{}/.../main.rs`", |
There was a problem hiding this comment.
A message like this should start with help: and start with a lowercase letter per rustc's style guide
| if path.is_dir() { | ||
| anyhow::bail!( | ||
| "path `{}` for {} `{}` is a directory, but a source file was expected.\n\ | ||
| Consider setting the path to the intended entrypoint file instead: `{}/.../main.rs`", |
There was a problem hiding this comment.
While Consider isn't a question, I feel like it falls into a similar category as questions in the rustc style guide
|
Thanks for your work on this! |
|
Closing this for now. I'll open a new one later with a more comprehensive rework for all target types. Thanks for being patient with me! |
…16338) ### What does this PR try to resolve? Closes #10173 This PR adds early validation of target source paths with clearer error messages. For context, I tried to solve this issue before ( #16329 ) but ended up making an oversight, I would recommend reading [this](#10173 (comment)) to understand the issues faced in resolving this. ### So what changed in this PR? - Root `Target` `Units` are validated early on in the compilation pipeline when the `BuildContext` is generated [here](https://github.com/rust-lang/cargo/blob/e0dd406b88933e82136bbc9073e06d029db8da45/src/cargo/ops/cargo_compile/mod.rs#L177). This is right after command line arguments/manifest targets are parsed and just before the build tasks are spawned. - **Cases that are covered by this PR:** - If the target path is invalid and points to nothing. - If the target path is valid, but is a directory (not a file) - If the target path is a valid directory with a `main.rs`/`lib.rs` depending on the Target kind: In this case a `help:` message is emitted. Doing this also means that Cargo validates each root target's path that is requested to be compiled before spawning any build tasks. This PR enforces the invariant: No compilation occurs before validating every required target's source path. I'm not sure if this should be expected behaviour and I'd love to hear from others on this.
2 participants
What does this PR try to resolve?
Addresses #10173
When directory paths are specified for example targets in Cargo.toml, cargo emits unhelpful os errors:
This PR fixes this by adding a validation function that checks if the target path is a valid source file. If not, emits an instructive error message: